Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fontique] fontconfig: allow_dtd for xml config #44

Merged
merged 1 commit into from
Apr 26, 2024
Merged

[fontique] fontconfig: allow_dtd for xml config #44

merged 1 commit into from
Apr 26, 2024

Conversation

dfrg
Copy link
Collaborator

@dfrg dfrg commented Apr 26, 2024

Appears to fix the issue with the roxmltree update.

@DJMcNab
Copy link
Member

DJMcNab commented Apr 26, 2024

The commit used in linebender/xilem#224 (4e6109f) doesn't match the head commit of this PR (3dd6f9b)

Indeed, 4e6109f is apparently 3 weeks old?

allow_dtd: true,
nodes_limit: u32::MAX,
},
) else {
return;
Copy link
Member

@DJMcNab DJMcNab Apr 26, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having any error handling here is a bit weird - there's a real difference between "the file doesn't exist, so try the next file" (?), and "the file does exist but is malformed XML"

Although I see that's a pre-existing choice

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I don't have a great answer for this. We can surface some sort of error which I suppose would have helped us catch this particular bug but in the general case, there's no real way to guard against the user having either a broken font configuration or one that we're incapable of reading.

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving based on xilem#224

My previous comments still stand, but are not blocking

@dfrg
Copy link
Collaborator Author

dfrg commented Apr 26, 2024

Thanks for catching the bad rev. I have no idea how I ended up with that value.

@dfrg dfrg added this pull request to the merge queue Apr 26, 2024
Merged via the queue into main with commit ddb0c6e Apr 26, 2024
19 checks passed
@dfrg dfrg deleted the allow_dtd branch April 26, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants